Use PolarisImmutable for StorageCredentialCacheKey #2029
Conversation
XN137
force-pushed
the
immutable-StorageCredentialCacheKey
branch
2 times, most recently
from
39eac51 to
a23c4a0
Compare
| + ", allowedWriteLocations=" | ||
| + allowedWriteLocations | ||
| + '}'; | ||
| return ImmutableStorageCredentialCacheKey.builder() |
There was a problem hiding this comment.
Why not delegate to the existing ImmutableStorageCredentialCacheKey.of(...) ?
There was a problem hiding this comment.
it didnt exist until i added @Value.Parameter, but done now
There was a problem hiding this comment.
Yea - this, rather the whole change, is overall be a good improvement not just for the code, but also the "free" improvements like lazy/memoized hash values.
XN137
force-pushed
the
immutable-StorageCredentialCacheKey
branch
from
a23c4a0 to
b97764f
Compare
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| /** | ||
| * The entity id is passed to be used to fetch subscoped creds, but is not used to do hash/equals | ||
| * as part of the cache key. | ||
| */ |
There was a problem hiding this comment.
I know that this PR does not change this behaviour, but conceptually if the entity ID is a substantial parameter to computing the cached value, I believe it should be included in the key.
There was a problem hiding this comment.
Not sure whether I already mentioned it, but I'm not sure the StorageCredentialCache works as expected, because when credentials are retrieved from the cache, we only know that those are valid at the time of retrieval, but not how long those will be valid (for a client), it could be 0.1ms or 30days.
There was a problem hiding this comment.
(But that's not a problem of this PR)
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
note that
entityIdwas not part of theequals/hashCodeand thus did not need to be a field